Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase API request timeout to 30s #347

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bfhealy
Copy link
Contributor

@bfhealy bfhealy commented Dec 21, 2022

This PR increases the timeout for API requests to 30s.

Copy link
Collaborator

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bfhealy There are a few fail_timeout's in here.

@stefanv
Copy link
Contributor

stefanv commented Dec 21, 2022

Please read https://nginx.org/en/docs/http/ngx_http_upstream_module.html#fail_timeout. Specifically, note that making the timeout higher sets both detection of timeout and the amount of time the server is considered offline before retrying connection.

@mcoughlin
Copy link
Collaborator

@stefanv We have issues with the permissions checking causing the plotting to timeout (and some reasonable queries also being a bit on the slower side).

@stefanv
Copy link
Contributor

stefanv commented Dec 21, 2022

All I'm saying is that this may not be the silver bullet you need, since while it will tolerate longer timeouts, it will also cause nodes to be offline for longer.

If possible, you can make the slow endpoints async or add more endpoints (might require more CPUs).

@mcoughlin
Copy link
Collaborator

@stefanv It's true neither the source nor the plot handlers are async currently. Do you want to try that first @bfhealy ?

@bfhealy
Copy link
Contributor Author

bfhealy commented Dec 21, 2022

@mcoughlin Sure, I'll take a look at making that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants